Skip to content

fix: put the prometheus scrape endpoint under the drain - #692

Merged
kacy merged 1 commit into
mainfrom
fix-prometheus-shutdown
Aug 10, 2026
Merged

fix: put the prometheus scrape endpoint under the drain#692
kacy merged 1 commit into
mainfrom
fix-prometheus-shutdown

Conversation

@kacy

@kacy kacy commented Aug 10, 2026

Copy link
Copy Markdown
Owner

the /metrics endpoint was the one accept loop in std that a graceful shutdown
could not see. serve() bound its listener without registering it, looped on
while true, and spawned each scrape without taking a drain count — so a
shutdown request left the port accepting, and a scrape already mid-response was
invisible to drain_default(), which reported a clean shutdown over it. because
the loop never exited, its defer tcp_close(fd) was reachable only on the
back-off give-up path: the listener was never closed on shutdown at all.

the loop now follows the shape #691 settled on for the six http/2 and web loops.
the listener is registered at the bind, so request() can stop it accepting and
close_listener owns the close exactly once. the loop tests the shutdown flag at
the top and again on a failed accept. spawn_scrape takes the concurrency permit
and the drain count together, on the accept loop, and serve_scrape releases
both through defer — including on the path where the scrape dies before its
request head arrives. the port is freed before the drain rather than after, and
serve() returns the drain result instead of an unreachable 0.

nothing is released after the drain, because a scrape has no equivalent of the
tls config binding that #691 tripped over. it borrows only its own socket, which
it owns, and the process-wide metric registry, which serve() does not own and
does not tear down. the Semaphore bound the loop already had is unchanged.

testing the flag at the top of the loop turned out to be load-bearing rather than
cosmetic: a listener bound after request() has run is not in the registry
request() walked, so nothing will ever shut it down, and a loop that went
straight into accept() would park there for the rest of the process's life.
that is a real race for spawn prometheus.serve(...) against a SIGTERM at
startup, and it has its own test.

what was tested

the bug first, on both backends. a scrape parked mid-request (its head sent, its
blank line never), then a shutdown request and a drain:

listeners registered: 0
inflight with a scrape parked mid-request: 0
drain_default returned 0 after 0ms
port still accepts after the drain: true

byte-identical under PITH_GREEN=0 and PITH_GREEN=1: nothing registered,
nothing counted, a clean drain reported instantly over a scrape still holding a
socket, and the port still answering. after the fix, the same program:

listeners registered: 1
inflight with a scrape parked mid-request: 1
drain_default returned 0 after 1709ms
port still accepts after the drain: false

four new colocated tests in std/prometheus.pith, all of them ordered off
observable state rather than off a sleep:

  • an accepted scrape is in the drain count before its task runs. eight real
    socket pairs are opened against a listener bound first, handed to
    spawn_scrape by hand, and the count read in exactly the window the bug lived
    in: inflight() == 8 the instant the last hand-off returns. the peers stay
    open and silent, so no task can reach its leave and the number is exact
    rather than racy — drain(20) == 8 then shows the drain giving up at its
    deadline and reporting the work rather than claiming a clean shutdown. closing
    the peers gives drain(5000) == 0 and inflight() == 0: every task leaves
    exactly once, on the path where the scrape dies before its request head.
  • serve registers its listener and closes it on shutdown. request() ends
    the accept loop on its own, serve() returns 0, listeners() is back to 0 and
    a fresh connect to the port is refused.
  • serve started after a shutdown request never accepts. the flag is set
    before the bind, and serve() returns without ever parking in accept().
  • a scrape leaves the drain count exactly once, answered or aborted. a scrape
    answered in full and one that dies before sending a byte both settle back to
    inflight() == 0. the drain rather than a bare read, because the count is
    released in a defer that need not have run when the client holds the body;
    and inflight() checked separately afterwards, because a double leave drives
    the count negative and drain() reports that as 0 too.

each was falsified, on both backends. with enter() moved back inside
serve_scrape, only the accept-time test fails, and identically either way:

PITH_GREEN=0  assertion failed: 7 != 8    6 passed, 1 failed
PITH_GREEN=1  assertion failed: 7 != 8    6 passed, 1 failed

with register_listener dropped, 3 passed, 4 failed on both. and with while not shutdown.requested() reverted to while true, the file hangs until the
harness kills it (exit 143 under PITH_GREEN=0 and =1) — the "never accepts"
test catching exactly the park it exists for.

with the fix in place, 7 passed, 0 failed on both backends, and 15 consecutive
runs of the file on each with no failures.

make run-regressions-only: 332 passed, 0 failed.

two test-only flakes fixed on the way past

test "serve answers several concurrent scrapes" failed once in about a dozen
runs under PITH_GREEN=1 before any of this. it waited time.delay(200) for the
listener to bind, which on a loaded two-core box is not always enough — the
client then fails to connect and the assertion fails on an empty body. it now
waits on shutdown.listeners() instead, which is only possible because serve()
registers its listener now. the same helper is what the new tests use, so none of
them sleeps.

raw_scrape also judged a response by a single read, so a response split
across two segments would have been assessed on its first. it now reads to the
end of the connection, which the server closes when it is done.

unrelated, and left alone

under PITH_GREEN=1 a socket read timeout does not bound a parked read. the
scrape in the reproduction above holds its tcp_set_timeout(fd, 2000) and still
sits there: on os threads the drain finished in 1709ms, under green the same
scrape held the count for a 4000ms grace period and then a 12000ms one, both to
the millisecond. fdio.rs's green read path calls wait_ready(fd, false, -1)
an unbounded reactor wait — where the os-thread path relies on SO_RCVTIMEO. so
every tcp_set_timeout guard in std is inert under the green runtime. it is
pre-existing, it is the safe direction here (the work is reported by the drain
rather than dropped), and it is not this change's to fix.

`serve()` ran an accept loop that std.shutdown could not see. it never
registered its listener, so a shutdown request left the port accepting; it
looped on `while true`, so it never returned and its `defer tcp_close(fd)` only
ever ran on the back-off give-up path; and it took no drain count, so a scrape
already in flight was invisible and `drain_default()` reported a clean shutdown
over a response being written.

the loop now follows the shape the six http/2 and web loops settled on: register
the listener at the bind, test the shutdown flag at the top of the loop and on a
failed accept, take the drain count on the accept loop rather than inside the
spawned task, free the port before draining, and return the drain result.
`serve_scrape` releases that count through `defer`, alongside the permit it
already released. nothing is released after the drain: unlike a tls listener, a
scrape borrows nothing from the listener — only its own socket and the
process-wide metric registry, neither of which this call owns.
@kacy
kacy merged commit 15d9915 into main Aug 10, 2026
2 checks passed
@kacy
kacy deleted the fix-prometheus-shutdown branch August 10, 2026 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant